fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees - #855

internal/worktrees/worktrees_posix.go (1)

20-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Default unknown signal errors to alive, so the fail-closed contract holds.

processAlive documents that any ambiguity counts as alive, so an uncertain answer can never expire a lease. This implementation returns alive only for nil and EPERM. Every other error, including an unexpected one, returns false and marks the lease expired. That path lets Clean unlock and force-remove a worktree based on an inconclusive probe.

Treat only the definite "no such process" answers as dead.

🛡️ Proposed fix

 	err = proc.Signal(syscall.Signal(0))
 	if err == nil {
 		return true
 	}
-	if errors.Is(err, os.ErrProcessDone) {
+	// Only ESRCH / ErrProcessDone prove the PID names no live process. Any
+	// other error is inconclusive, so report alive and keep the lease.
+	if errors.Is(err, os.ErrProcessDone) || errors.Is(err, syscall.ESRCH) {
 		return false
 	}
-	return errors.Is(err, syscall.EPERM)
+	return true
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees_posix.go` around lines 20 - 27, Update the error
handling in processAlive so only the definite no-process result
(os.ErrProcessDone) returns false; retain true for successful probes and
permission errors, and default all other unexpected signal errors to true to
preserve the fail-closed lease behavior.

internal/cli/workflow_test.go (2)

325-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Two secret scanners flag these synthetic key literals.

The literals are clearly fake fixtures for the redaction assertions, and the tests are valuable. Both Betterleaks and ast-grep report them as hard-coded credentials, which adds recurring noise to every scan of this file. Assemble the fixture at runtime from fragments, or add the scanners' inline allow annotation, so the finding does not have to be triaged again.

♻️ Proposed fix

-	secret := "sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu"
+	// Assembled at runtime so secret scanners do not report this synthetic
+	// redaction fixture as a hard-coded credential.
+	secret := "sk-" + "proj-abcDEF123_ghiJKL456-mnoPQR789stu"

Also applies to: 933-933

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` at line 325, Update the synthetic secret
fixtures in the workflow tests, including both occurrences of the key literal,
so scanners no longer classify them as hard-coded credentials. Assemble each
fixture at runtime from non-secret fragments, or apply the repository-supported
inline allow annotation while preserving the existing redaction assertions.

Source: Linters/SAST tools

---

187-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use t.Chdir for the workflow test temporary directory.

go.mod declares Go 1.26.5, so testing.T.Chdir is available. Use it instead of manual os.Chdir plus deferred restore; it restores the working directory automatically during cleanup.

♻️ Proposed fix

-	origWd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
 	parent := t.TempDir()
 	worktreeDir := filepath.Join(parent, "task-a")
 	if err := os.Mkdir(worktreeDir, 0o700); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chdir(parent); err != nil {
-		t.Fatal(err)
-	}
-	defer func() {
-		if err := os.Chdir(origWd); err != nil {
-			t.Fatal(err)
-		}
-	}()
+	t.Chdir(parent)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 187 - 203, Update the workflow
test’s working-directory setup to use testing.T.Chdir with the temporary parent
directory, and remove the manual os.Getwd, os.Chdir, and deferred restoration
logic. Preserve the existing worktreeDir creation and test behavior.

internal/cli/workflows.go (1)

133-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Three small CLI UX inconsistencies in the release path.

1. Line 180 prints the raw path argument, but line 177 releases absPath. A user who runs zero worktrees release task-a sees released task-a while git unlocked an absolute path. Report the path that was acted on.
2. Line 134 does not trim the --cwd= value, while parseWorktreeCommandArgs trims the same flag at line 405. --cwd=" " then reaches resolveWorkspaceRoot as whitespace.
3. In the help text, -h, --help is now the last entry of the release flags: block, so it reads as release-only. It applies to both subcommands.

♻️ Proposed fixes

 		case strings.HasPrefix(arg, "--cwd="):
-			cwdFlag = strings.TrimPrefix(arg, "--cwd=")
+			cwdFlag = strings.TrimSpace(strings.TrimPrefix(arg, "--cwd="))

-	if _, err := fmt.Fprintf(stdout, "released %s\n", redactCLIString(path)); err != nil {
+	if _, err := fmt.Fprintf(stdout, "released %s\n", redactCLIString(absPath)); err != nil {
 		return exitCrash
 	}

 release flags:
   -C, --cwd <path>        Source repository directory (required if the
                            worktree directory was already deleted)
+
+common flags:
   -h, --help              Show this help

Also applies to: 180-182

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 133 - 134, Update the release
workflow to report the acted-on absolute path variable (absPath) instead of the
raw path argument, trim whitespace from the --cwd= value when assigning cwdFlag
in argument parsing, and move the -h, --help entry in the release flags help
text so it clearly applies to both release subcommands.

internal/worktrees/worktrees_test.go (1)

1268-1272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Two doc comments describe tests that are not below them.

At lines 1268-1272 a comment about --ignored blocking force-removal sits directly above TestCleanPreservesUnreachableCommitBeforeRemoval, which tests commit preservation. At lines 1547-1553 a comment about worktreeIsDirty and --ignored sits above TestCleanHonorsTouchLiveness. It looks like the tests those paragraphs belonged to moved. Move each paragraph onto its own test or delete it, so the next reader does not trust the wrong description.

Also applies to: 1547-1553

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees_test.go` around lines 1268 - 1272, Correct the
misplaced doc comments in internal/worktrees/worktrees_test.go: move the
--ignored/worktreeIsDirty force-removal explanation to the test that verifies
ignored-only content is treated as dirty, and move the commit-preservation
explanation to TestCleanPreservesUnreachableCommitBeforeRemoval. Ensure each
paragraph directly precedes the test it describes, or remove it if no matching
test exists.

internal/worktrees/worktrees.go (1)

284-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Use a locale-stable signal for git worktree lock collisions.

lockWorktree treats already locked as a non-lock failure, but that message is translated under LANG/LC_MESSAGES. A localized message can cause Prepare to classify a live worktree collision as a real failure and roll back/remove the newly created worktree. Run the lock through defaultRunGit with LC_ALL=C, or use a locale-independent signal such as the lock exit code plus another stable Git state (for example, querying the worktree list before running add).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees.go` around lines 284 - 294, Update lockWorktree
to classify existing-lock collisions using a locale-stable signal instead of
matching the localized “already locked” text. Run git worktree lock through
defaultRunGit with LC_ALL=C, or use the lock exit code together with another
stable Git-state check, while preserving successful locks and genuine error
propagation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/worktrees/worktrees_test.go (1)

1942-1981: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a failure case for the unlock in this path.

This test asserts that a dead-lease entry with a missing directory is unlocked and pruned. It only covers the success path. The source discards the unlock error here (see my comment on worktrees.go lines 880-888), so no test can currently detect that regression.

When you fix the source, add a companion case where the unlock returns a nonzero exit and assert that Clean returns an error naming the entry.

As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees_test.go` around lines 1942 - 1981, Add a
companion test alongside TestCleanUnlocksExpiredLeaseBeforePruningMissingDir
where the fakeRunner returns a nonzero exit for the unlock command, then assert
Clean returns an error that identifies the affected missing-dead-task entry.
Keep the existing success-path test unchanged and ensure the failure case
verifies the error is propagated rather than proceeding silently.

Source: Coding guidelines

internal/cli/workflow_test.go (3)

231-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two failure branches of runWorktreesRelease are untested.

The parser rejects a duplicate positional path with "worktree path was provided more than once" (workflows.go line 139) and an unrecognized flag with "unknown worktrees release flag" (line 136). Neither has a test. Both are cheap to add alongside this missing-path case.

As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."

💚 Cover the remaining parser rejections

func TestRunWorktreesReleaseRejectsDuplicatePath(t *testing.T) {
	var stdout, stderr bytes.Buffer
	exitCode := runWithDeps([]string{"worktrees", "release", "/a", "/b"}, &stdout, &stderr, appDeps{
		releaseWorktree: func(context.Context, worktrees.Options, string) error {
			t.Fatal("releaseWorktree must not run for an ambiguous path argument")
			return nil
		},
	})

	if exitCode != exitUsage {
		t.Fatalf("expected usage exit %d, got %d", exitUsage, exitCode)
	}
	if !strings.Contains(stderr.String(), "more than once") {
		t.Fatalf("expected duplicate-path error, got %q", stderr.String())
	}
}

func TestRunWorktreesReleaseRejectsUnknownFlag(t *testing.T) {
	var stdout, stderr bytes.Buffer
	exitCode := runWithDeps([]string{"worktrees", "release", "--nope", "/a"}, &stdout, &stderr, appDeps{
		releaseWorktree: func(context.Context, worktrees.Options, string) error {
			t.Fatal("releaseWorktree must not run for an unknown flag")
			return nil
		},
	})

	if exitCode != exitUsage {
		t.Fatalf("expected usage exit %d, got %d", exitUsage, exitCode)
	}
	if !strings.Contains(stderr.String(), "unknown worktrees release flag") {
		t.Fatalf("expected unknown-flag error, got %q", stderr.String())
	}
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 231 - 247, Add regression tests
alongside TestRunWorktreesReleaseRequiresPath for runWorktreesRelease parser
failures: verify duplicate positional paths return exitUsage, emit the “more
than once” error, and do not call releaseWorktree; also verify an unknown flag
returns exitUsage, emits the unknown-flag error, and does not call
releaseWorktree.

Source: Coding guidelines

---

813-853: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the Options.Cwd that exec passes to releaseWorktree.

exec.go line 224 deliberately passes trustRoot, the original launch directory captured before workspaceRoot is reassigned to the worktree path. That choice matters: Release uses Options.Cwd as git's working directory when the worktree directory is gone. If it were changed to workspaceRoot, git would run from inside the deleted worktree and the recovery path would break silently.

No test pins this. Add the assertion here, where the release callback already captures its arguments.

💚 Pin the trust-root wiring

 	root := t.TempDir()
 	worktreeDir := t.TempDir()
 	var releasedPath string
+	var releasedCwd string
 	releaseCalls := 0
@@
 		releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error {
 			releaseCalls++
 			releasedPath = path
+			releasedCwd = options.Cwd
 			return nil
 		},
@@
 	if releasedPath != worktreeDir {
 		t.Fatalf("released path = %q, want %q", releasedPath, worktreeDir)
 	}
+	// Release runs git from Options.Cwd when the worktree directory is gone,
+	// so exec must pass the original launch directory, not the worktree it
+	// reassigned workspaceRoot to.
+	if releasedCwd != root {
+		t.Fatalf("release Options.Cwd = %q, want the trust root %q", releasedCwd, root)
+	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 813 - 853, Extend
TestRunExecWorktreeReleasesLockAfterRun to capture the releaseWorktree options
argument and assert that Options.Cwd equals root, the original launch directory.
Keep the existing call-count and released-path assertions, and verify the
trust-root value passed by exec rather than the reassigned worktree workspace
path.

---

181-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use t.Chdir for the working-directory change.

go.mod declares Go 1.26.5, so t.Chdir(parent) is available and removes the manual os.Getwd/os.Chdir cleanup from this test.

♻️ Use `t.Chdir`

-	origWd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
 	parent := t.TempDir()
 	worktreeDir := filepath.Join(parent, "task-a")
 	if err := os.Mkdir(worktreeDir, 0o700); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chdir(parent); err != nil {
-		t.Fatal(err)
-	}
-	defer func() {
-		if err := os.Chdir(origWd); err != nil {
-			t.Fatal(err)
-		}
-	}()
+	t.Chdir(parent)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 181 - 229, Update
TestRunWorktreesReleaseNormalizesRelativePath to use t.Chdir(parent) instead of
manually capturing the current directory, calling os.Chdir, and restoring it
with defer. Remove the now-unnecessary origWd handling and cleanup while
preserving the test’s expected absolute-path behavior.

internal/worktrees/worktrees.go (1)

82-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The cleanup gate keys off an injected runner, which couples production behavior to test wiring.

options.RunGit == nil is true only for the default runner. Any embedder that injects its own GitRunner (a sandboxed or instrumented git wrapper) silently loses automatic stale cleanup, even though nothing about a custom runner makes cleanup unsafe. The real intent is "tests do not want the extra git calls".

Consider an explicit option (for example SkipAutoClean bool) so the behavior is named rather than inferred from which runner was supplied.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees.go` around lines 82 - 88, Replace the
options.RunGit == nil cleanup gate in the worktree request flow with an explicit
option such as SkipAutoClean. Run Clean with the existing 24-hour threshold by
default, including when a custom GitRunner is injected, and skip only when the
explicit test-oriented option is enabled.

internal/worktrees/worktrees_posix.go (1)

17-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The logic is correct, but this branch has no direct unit test while its Windows sibling does.

worktrees_windows_test.go covers the classification of each error case. There is no worktrees_posix_test.go. The ESRCH and EPERM branches are only exercised indirectly through Clean and Prepare.

Add a small POSIX test file that asserts the live-self case and the exited-child case, mirroring TestOsProcessAliveReportsLiveSelf and TestOsProcessAliveReportsDeadAfterExit.

As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths; path-sensitive logic must include a non-Linux case or a hermetic equivalent exercising the same normalization."

💚 Suggested `internal/worktrees/worktrees_posix_test.go`

//go:build !windows

package worktrees

import (
	"os"
	"os/exec"
	"testing"
)

func TestOsProcessAliveReportsLiveSelf(t *testing.T) {
	if !osProcessAlive(os.Getpid()) {
		t.Fatal("current process must report alive")
	}
}

func TestOsProcessAliveReportsDeadAfterExit(t *testing.T) {
	cmd := exec.Command("/bin/sh", "-c", "exit 0")
	if err := cmd.Start(); err != nil {
		t.Skipf("start child process: %v", err)
	}
	pid := cmd.Process.Pid
	if err := cmd.Wait(); err != nil {
		t.Fatalf("wait for child process: %v", err)
	}
	// Wait reaps the child, so the PID no longer names a process.
	if osProcessAlive(pid) {
		t.Fatal("exited and reaped process must not report alive")
	}
}

// PID 1 exists but is not signalable by an unprivileged caller, so the EPERM
// branch must classify it as alive rather than expiring a lease.
func TestOsProcessAliveTreatsPermissionDeniedAsAlive(t *testing.T) {
	if os.Geteuid() == 0 {
		t.Skip("running as root: signal 0 to pid 1 succeeds instead of returning EPERM")
	}
	if !osProcessAlive(1) {
		t.Fatal("a process we may not signal must be treated as alive")
	}
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/worktrees/worktrees_posix.go` around lines 17 - 32, Add a POSIX-only
test file covering osProcessAlive with the current process and a
started-then-reaped child, asserting alive and dead results respectively. Also
cover the permission-denied case for PID 1 when running unprivileged, skipping
under root, to exercise the EPERM classification. Mirror the existing Windows
test naming and use a portable child-process command.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes